Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update get_csa_header to return none on CSAReadError #501

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

parneshraniga
Copy link

Some non-MRI dicom datasets have just strings in CSA Header tags e.g Siemens PET dicoms . This causes the read on line 68 to throw an error because the string is not a CSA header. Unfortunately this throws out other packages such as dcmstack which use nibabel. The proposed fix returns none if a read error is encountered when such strings are parsed. It is possible such strings may pass the initial check for 0 < n_tags <=128 which throws the CSAReadError but fail later in which case the except block can be be modified to handle those exceptions as well.

Some non-MRI dicom datasets have just strings in CSA Header tags. This causes the read on line 68 to throw an error because the string is not a CSA header. Unfortunately this throws out other packages such as dcmstack which use nibabel. The proposed fix returns none if a read error is encountered when such strings are parsed. It is possible such strings may pass the initial check for 0 < n_tags <=128 which throws the CSAReadError but fail later in which case the except block can be be modified to handle those exceptions as well.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.992% when pulling f83ed47 on parneshraniga:master into 0f3048c on nipy:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.992% when pulling 4a73b36 on parneshraniga:master into 0f3048c on nipy:master.

@codecov-io
Copy link

codecov-io commented Jan 4, 2017

Current coverage is 94.02% (diff: 100%)

Merging #501 into master will not change coverage

@@             master       #501   diff @@
==========================================
  Files           166        166          
  Lines         21832      21832          
  Methods           0          0          
  Messages          0          0          
  Branches       2325       2325          
==========================================
  Hits          20527      20527          
  Misses          875        875          
  Partials        430        430          

Powered by Codecov. Last update 6104bd1...2c1d332

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.992% when pulling 920cf49 on parneshraniga:master into 0f3048c on nipy:master.

@matthew-brett
Copy link
Member

Thanks for this - have you got an example DICOM you can share, that we can use to test? Is there anything about the string that allows us to detect its presence in a more specific way than an error in the csa read routine?

@parneshraniga
Copy link
Author

Thus far it seems to fail for Siemens PET data. I will check to see if the data can be shared or is already available and let you know. On the two datasets that I checked, there wasn't anything in particular that stood out but I will go through the data I have to see what I can find.

@matthew-brett
Copy link
Member

You can delete the data in the DICOM file if you want, and replace it with zeros, so the file compresses well, and doesn't reveal private brain data.

@effigies
Copy link
Member

effigies commented Jan 6, 2017

@parneshraniga Can you merge master or rebase to fix Travis tests?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.992% when pulling 2c1d332 on parneshraniga:master into 6104bd1 on nipy:master.

@parneshraniga
Copy link
Author

Done.

@effigies
Copy link
Member

Great! Thanks. Any luck getting an anonymized DICOM that was failing before?

@parneshraniga
Copy link
Author

Yes I can grab a couple of files, anonymise them and clear the data out. Whats the best way to forward them to you?

@effigies
Copy link
Member

Any place we can grab them from. Google drive or dropbox are common. If they're small, they can go in nibabel/tests/data in a new commit; if they're big, see http://nipy.org/nibabel/devel/add_test_data.html.

@matthew-brett
Copy link
Member

Guys - now is an excellent time to get this one finished up - I have lots of free time at the moment to work on this.

@matthew-brett
Copy link
Member

@parneshraniga - would you mind sharing those files? That would be very helpful.

@matthew-brett
Copy link
Member

@parneshraniga - anything we can do to help? Can you give us access to the unmodified files? We can anonymize them for you.

@matthew-brett
Copy link
Member

@parneshraniga - another reminder in hope that we can get this one in.

@effigies
Copy link
Member

@parneshraniga Do you have any time to help get this one finished?

@effigies
Copy link
Member

Hi @parneshraniga, is there any chance you still have access to files that can reproduce this issue?

@effigies effigies added the bug label Mar 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants